💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3616b379f

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ac232a3205

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b10b4c77ef

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 158bba5602

Code Review

Overall: Thorough fix for Windows file link handling in the Markdown renderer. The refactoring extracts file link parsing into a dedicated utility module, which is a clear improvement over the scattered regex patterns. Excellent test coverage.

Strengths

1. Centralized parsing via fileLinks utility. Moving parseFileLocation, normalizeFileLinkPath, and fromFileUrl into a shared utility eliminates duplicated regex patterns (FILE_LINE_SUFFIX_PATTERN, FILE_HASH_LINE_SUFFIX_PATTERN, etc.) and makes the logic testable in isolation.

2. Windows path support is comprehensive. Handles drive letters (I:\path), UNC paths (\server\share), namespace-prefixed paths (\?\C:), and the #L anchor normalization (converting #L12 to :12). The test matrix covers all these variants.

3. Test coverage is excellent. 147+ new lines of Markdown.test.tsx covering Windows absolute paths in plain text, #L anchors in hrefs, workspace route exclusions, file:// URL edge cases, and percent-encoding corner cases. The useFileLinkOpener tests for Copy Link with namespace-prefixed paths are a nice addition.

Potential Issues

4. resolveHrefFilePath candidate loop: The new approach iterates over [url, safeDecodeURIComponent(url)] and checks isLikelyFileHref for each. This means double-decoding could occur if the URL is already decoded. The Set-based dedup handles the common case, but a URL like %2525 (double-encoded %) would decode to %25, then to %, creating a different path than intended. Low risk in practice since file paths rarely contain percent signs.

5. urlTransform early return for file links: Adding the resolveHrefFilePath check before the scheme-based checks in urlTransform is important. Without this, react-markdown's default URL sanitizer would strip Windows absolute paths (they look like unknown schemes). This is correct but worth a comment explaining why it's needed.

6. isKnownLocalWorkspaceRouteFilePath guard in getLinkablePath: Good catch preventing /workspace/settings#L12 from being treated as a file link. The test for inline code workspace routes confirms this works correctly.

7. title attribute addition on file links: Adding title={hrefFilePath} to the anchor element is a nice UX touch, showing the full resolved path on hover. This also serves as documentation of what path will be opened on click.

No bugs or security issues found. Clean refactoring with strong test coverage.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08f6339091

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21a0a65e3d

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9206362248

Code Review: Windows File Links in Messages

Quick check - can you share the diff?

Code Review: Windows File Links in Messages

Overall: Thorough work handling Windows paths (drive letters, backslashes, UNC paths) in the Markdown file-link resolver. Test coverage is excellent with 15+ new test cases covering edge cases.

Items:

1. The test 'keeps generic workspace routes as normal markdown links' was renamed to 'keeps exact workspace routes' and the test path changed from '/workspace/reviews/overview' to '/workspace/reviews'. This changes the test semantics - the original tested that nested routes under reserved names were preserved as local routes. Make sure the nested-route behavior is still covered (the new 'keeps nested reviews routes local' test covers the /workspaces/ variant but not the /workspace/ singular form with sub-paths).

2. Windows path linkification with unescaped percent signs (the '100%.tsx' tests) is handled gracefully, which is good. However, verify that the percent-encoding roundtrip is consistent - the href shows '100%25.tsx' but onOpenFileLink receives '100%.tsx'. If any downstream code re-encodes the path, you could end up with double-encoding.

3. The file:// URL handling strips non-line fragments (#overview becomes no suffix) but preserves #L fragments (#L12 becomes :12). This is the right behavior. However, the test 'ignores non-line file URL fragments' verifies the callback receives '/tmp/report.md' without the fragment. Make sure this is consistent with how the editor actually opens files - some editors support anchor-based navigation.

4. UNC path support (file://server/share/...) properly resolves to '//server/share/...' which is correct for Windows. No issues.

Tests: Exceptional coverage - drive letters, UNC paths, percent-encoded filenames, #L anchors, workspace route disambiguation, inline code exclusion. Well structured.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb3f373f1c

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 136b706c38